Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: zetaclient should pick up new connector and erc20 custody addresses #1890

Merged
merged 6 commits into from
Mar 19, 2024
Merged

fix: zetaclient should pick up new connector and erc20 custody addresses #1890

merged 6 commits into from
Mar 19, 2024

Conversation

ws4charlie
Copy link
Contributor

@ws4charlie ws4charlie commented Mar 13, 2024 •
edited
Loading

Description

  1. Zetaclient should be able to refresh connector/erc20Custody addresses at runtime.
  2. Added unit tests.

Closes: 1633

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Checklist:

  • I have added unit tests that prove my fix feature works

@codecov Codecov
Copy link

codecov bot commented Mar 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 77.21519% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 48.14%. Comparing base (f2fde15) to head (70e541e).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1890      +/-   ##
===========================================
- Coverage    48.55%   48.14%   -0.42%     
===========================================
  Files          232      233       +1     
  Lines        13098    13317     +219     
===========================================
+ Hits          6360     6411      +51     
- Misses        6296     6464     +168     
  Partials       442      442
Files Coverage Δ
zetaclient/bitcoin/bitcoin_client.go 13.30% <ø> (+0.01%) ⬆️
zetaclient/evm/evm_client.go 14.85% <ø> (ø)
zetaclient/evm/evm_signer.go 47.93% <92.10%> (+2.09%) ⬆️
zetaclient/bitcoin/bitcoin_signer.go 0.00% <0.00%> (ø)
zetaclient/zetacore_observer.go 18.09% <74.28%> (ø)

@ws4charlie
Merge branch 'develop' of https://github.com/zeta-chain/node into fix…
77c91cc 
…-zetaclient-update-connector-erc20custody-address
lumtis
lumtis reviewed Mar 14, 2024
View reviewed changes
zetaclient/evm/evm_signer.go Show resolved Hide resolved
zetaclient/evm/evm_signer.go Show resolved Hide resolved
zetaclient/zetacore_observer.go Outdated Show resolved Hide resolved
zetaclient/zetacore_observer.go Outdated Show resolved Hide resolved
zetaclient/zetacore_observer.go Show resolved Hide resolved
@lumtis
Copy link
Member

lumtis commented Mar 14, 2024

@ws4charlie now the client and signer for each chain update dynamically the chain params?

Can we confirm the following?

  • If new chain support is setup but no activated yet isSupported=false, the ZetaClient will actively start supporting the chain when isSupported is changed to true in the chain param and we don't need to restart ZetaClient
  • If we want to remove support for a chain, we can simply set isSupported to false in the chain param and ZetaClient will automatically stop interacting with the chain without the need to be restarted

@ws4charlie
Copy link
Contributor Author

ws4charlie commented Mar 15, 2024 •
edited
Loading

@ws4charlie now the client and signer for each chain update dynamically the chain params?

Can we confirm the following?

  • If new chain support is setup but no activated yet isSupported=false, the ZetaClient will actively start supporting the chain when isSupported is changed to true in the chain param and we don't need to restart ZetaClient
  • If we want to remove support for a chain, we can simply set isSupported to false in the chain param and ZetaClient will automatically stop interacting with the chain without the need to be restarted
  1. Dynamically pause/unpause a connected chain is tracked by this issue: zetaclient should check 'IsSupported' flag to be able to pause/unpause a specific chain #1883
  2. Dynamically add a new chain is a manual process which will at least involve
    • Observers runners update their zetaclient config files and zetaclient should be able to take new config file dynamically.
    • Zetaclient should be able to detect newly added chain with IsSupported flag enabled.
    • Zetaclient should also be able to dynamically create new ChainClient and ChainSigner objects and start new threads.

@lumtis
Copy link
Member

lumtis commented Mar 15, 2024 •
edited
Loading

Dynamically add a new chain is a manual process which will at least involve
Observers runners update their zetaclient config files and zetaclient should be able to take new config file dynamically.
Zetaclient should be able to detect newly added chain with IsSupported flag enabled.
Zetaclient should also be able to dynamically create new ChainClient and ChainSigner objects and start new threads.

We should implement all of this. Let's track issues if those don't exist

@ws4charlie
Copy link
Contributor Author

Dynamically add a new chain is a manual process which will at least involve
Observers runners update their zetaclient config files and zetaclient should be able to take new config file dynamically.
Zetaclient should be able to detect newly added chain with IsSupported flag enabled.
Zetaclient should also be able to dynamically create new ChainClient and ChainSigner objects and start new threads.

We should implement all of this. Let's track issues if those don't exist

#1904

lumtis
lumtis approved these changes Mar 16, 2024
View reviewed changes
Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

One comment, and need to address build failure

zetaclient/testutils/constant.go Show resolved Hide resolved
kevinssgh
kevinssgh approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@kevinssgh kevinssgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@ws4charlie ws4charlie merged commit 337e485 into develop Mar 19, 2024
21 checks passed
@ws4charlie ws4charlie deleted the fix-zetaclient-update-connector-erc20custody-address branch March 19, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinssgh kevinssgh kevinssgh approved these changes

@lumtis lumtis lumtis approved these changes

@brewmaster012 brewmaster012 Awaiting requested review from brewmaster012

@kingpinXD kingpinXD Awaiting requested review from kingpinXD

@fadeev fadeev Awaiting requested review from fadeev

@skosito skosito Awaiting requested review from skosito

Assignees
No one assigned
Labels
breaking:cli
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

zetaclients should be able to pick up update on Connector/ERC20Custody addresse
3 participants
@ws4charlie @lumtis @kevinssgh